Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't silently ignore creation of new windows #885

Merged
merged 1 commit into from
Jun 18, 2020
Merged

Don't silently ignore creation of new windows #885

merged 1 commit into from
Jun 18, 2020
+47 −1

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Jun 2, 2020

Platforms affected

iOS

Motivation and Context

Currently, attempts to create new windows (via links with target="_blank" or JS window.open) for pages within the application have no effect and are silently ignored. (Links to pages outside the application trigger the allow-intent functionality and open in Safari.)

Description

With this PR, the default behaviour will be to open those links in the same Web View (without creating a new window).

This also introduces a preference (AllowNewWindows) that allows the creation of a new UIViewController to present the new page in a new Web View frame on top of the existing Web View.
Note: This new window provides no controls, so it can only be closed with window.close() in JS.

Testing

Manual testing in a new Cordova app.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • I've updated the documentation if necessary

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2020 •
edited
Loading

Codecov Report

Merging #885 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #885   +/-   ##
=======================================
  Coverage   74.40%   74.40%           
=======================================
  Files          13       13           
  Lines        1676     1676           
=======================================
  Hits         1247     1247           
  Misses        429      429

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02448ce...5d6d780. Read the comment docs.

@dpogue dpogue marked this pull request as ready for review June 10, 2020 06:28
erisu
erisu approved these changes Jun 17, 2020
View reviewed changes
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM.
I also tested

  • building
  • ensure the default behavior opens link
  • AllowNewWindows set to true 
     <a href="test.html target="_blank">
    and 
    window.open('test.html', '_blank');
    opens a new window and window.close will close the new window.

@dpogue
Don't silently ignore creation of new windows
35e017c 
Currently, attempts to create new windows (via links with
`target="_blank"` or JS `window.open`) for pages within the application
have no effect and are silently ignored.
(Links to pages outside the application trigger the allow-intent
functionality and open in Safari.)

With this PR, the default behaviour will be to open those links in the
**same** Web View (without creating a new window).

This also introduces a preference (`AllowNewWindows`) that allows the
creation of a new UIViewController to present the new page in a new Web
View frame on top of the existing Web View.
**Note:** This new window provides no controls, so it can only be closed
with `window.close()` in JS.
@dpogue dpogue merged commit 32b21e1 into apache:master Jun 18, 2020
@dpogue dpogue deleted the target-blank branch June 18, 2020 15:16
@dpogue dpogue added this to the 6.1.0 milestone Jun 18, 2020
@dpogue dpogue mentioned this pull request Jun 23, 2020
Merged
@vbraun
Copy link
Contributor

vbraun commented Oct 18, 2021

What about iframes with target="_blank" links? These now either replace the app, or open another window on top without any way for the app to close it.

E.g. embedded google maps has a target="_blank" link to open the full screen map. We want to embed it into our app as iframe, so <allow-navigation href="https://*.google.com/*" /> must be set or iOS displays nothing. Then user clicks on the link in the iframe. Used to just not do anything (bad), now just replaces the app (worse).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erisu erisu erisu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@dpogue @codecov-commenter @vbraun @erisu